perf: Cache HTTP requests across all pages#174
Conversation
| @@ -214,6 +218,7 @@ const createCheckAliveURL = (ruleOptions: Options, resolvePath: (path: string, b | |||
| const finalRes = await fetchWithDefaults(redirectedUrl, { ...opts, redirect: "follow" }); | |||
| const url = URL.parse(uri); | |||
| const hash = url?.hash || null; | |||
| await finalRes.body?.cancel(); | |||
| return { | |||
| ok: finalRes.ok, | |||
| redirected: true, | |||
| @@ -264,6 +269,10 @@ const createCheckAliveURL = (ruleOptions: Options, resolvePath: (path: string, b | |||
| ok: false, | |||
| message: ex.message | |||
| }; | |||
| } finally { | |||
| if (res && !res.body?.locked) { | |||
| await res.body?.cancel(); | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Is it okay to understand that this change has nothing to do with Cache?
https://github.com/nodejs/undici?tab=readme-ov-file#garbage-collection
I feel like it's good to explicitly consume because you can't rely on GC with Node fetch
There was a problem hiding this comment.
No, in my environment, after placing the cache globally, it started hanging occasionally. Therefore, I believe it is appropriate to include this change in this PR.
| const memorizedIsAliveURIByOptions = new Map< | ||
| string, | ||
| ReturnType<typeof pMemoize<ReturnType<typeof createCheckAliveURL>>> | ||
| >(); |
There was a problem hiding this comment.
If you have a Cache Map globally, you will behave like a memory leak if you don't condition it by size or TTL like LRU Cache.
CLI is not a big problem, but if you run it in a server-like shape or editor, it will always refer to the same global cache, so I think memory consumption will increase linearly
There was a problem hiding this comment.
The function pMemoize allows setting the cache TTL. Since the rule textlint-rule-no-dead-link sets it to 30 seconds by default, I believe there is no need to implement TTL again. What do you think?
There was a problem hiding this comment.
📝 memorizedIsAliveURIByOptions is still global variables.
|
@azu |
| const memorizedIsAliveURIByOptions = new Map< | ||
| string, | ||
| ReturnType<typeof pMemoize<ReturnType<typeof createCheckAliveURL>>> | ||
| >(); |
There was a problem hiding this comment.
📝 memorizedIsAliveURIByOptions is still global variables.
azu
added
the
Type: Refactoring
2 participants
Background
This package caches HTTP requests used to verify link existence. However, since the cache was created per page, duplicate links across pages were causing cache misses.
Approach
I changed the caching to be global, not per page.
I also modified the cache key to include the rule's options. While actual use assumes options don't vary per page, test cases may have different options during testing.
Additionally, the rule with these changes occasionally hung, so I addressed this by always consuming
Response.body.